Skip to content

Comments

linux watcher bug fixes and general slight improvements#4

Closed
whyvn wants to merge 6 commits intofreref:masterfrom
whyvn:callback-from-main
Closed

linux watcher bug fixes and general slight improvements#4
whyvn wants to merge 6 commits intofreref:masterfrom
whyvn:callback-from-main

Conversation

@whyvn
Copy link
Contributor

@whyvn whyvn commented Jan 8, 2025

resolves #3

if you do not wish to merge this pr the bug fixes can still be merged

syntax/compilation errors in `removeFile`, and remove `offset` since its information is stored in `self.paths.items.len`
@whyvn whyvn marked this pull request as draft January 8, 2025 02:48
whyvn added 3 commits January 8, 2025 16:06
doesnt compile but the idea is that it would be more safe to update the watch descriptors of the previous files so that we can check valid descriptors in the event loop
i think its safe not to even check if the wd is below offset, it would be much faster and if inotify is sending it than it must already be valid. ill leave this commit as the basis for an idea though
creates a bit of a bug where the wd will only be updated once another file is modified
@whyvn
Copy link
Contributor Author

whyvn commented Jan 8, 2025

1433508
and
f077375

are ideas for "invalid" watch descriptors although i couldn't get them to work (more information is provided in the commit paragraphs). why does inotify ignore updating future wds even if you remove a watch target :(

all commits up until here have just been bug fixes (and changing Event but i think that's pretty useful)

also someone must verify that the macos build is fine

@freref
Copy link
Owner

freref commented Jan 9, 2025

Hi Julia, thanks for your contributions, I will take a look at this after my finals as I would also have to adjust the macos build

@whyvn
Copy link
Contributor Author

whyvn commented Jan 9, 2025

oh dont worry about it yet. i still need to implement the actual feature of this pr and i will make the draft ready once i get home and do that!

@whyvn
Copy link
Contributor Author

whyvn commented Jan 10, 2025

Ok, ive come to the conclusion that, although i can see a use for this idea, its too niche to warrant extending the api in such a way that makes it extremely clunky (e.g. a hypothetical Watcher.poll() would have to reset Watcher.modified to prevent a race condition from the main event loop however what if the user wants to call poll multiple times? or what if they want to check modified without poll?)

also, the way it would work is not too far off with the user implementing it themselves like in #3. you still have to call poll in the main event loop and call the function from there unless we do fancy meta programming so yeah. ill just keep this pr as bug fixes i guess.

also, ive changed Event to include an item field which i think would be useful.

@whyvn whyvn mentioned this pull request Jan 10, 2025
@whyvn whyvn changed the title callback from main thread linux watcher bug fixes and general slight improvements Jan 10, 2025
@whyvn whyvn marked this pull request as ready for review January 10, 2025 03:19
remove artifact from poll testing
@freref
Copy link
Owner

freref commented Feb 19, 2025

Closing in favor of #5 which continues this work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

callback from main thread

2 participants